Skip to content

✨ feat(reviewer-lane): ship the adjudication contract as a template (#5617 item 2) - #5720

Merged
clubanderson merged 2 commits into
hivecommons:v5from
Danathar:feat/5617-reviewer-lane-template
Sep 3, 2026
Merged

✨ feat(reviewer-lane): ship the adjudication contract as a template (#5617 item 2)#5720
clubanderson merged 2 commits into
hivecommons:v5from
Danathar:feat/5617-reviewer-lane-template

Conversation

@Danathar

@Danathar Danathar commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Item 2 of #5617 — the embedded kick template — which is the last of the four deferred follow-ups still outstanding, and the one @clubanderson's dispatch on the issue asked for. The reviewer lane's adjudication contract moves from a compiled-in string into pkg/policies/defaults/reviewer-lane.md.

The lane reaches its prompt by role, not by kick_template: an operator enables it by adding a cadence agent with role: reviewer — any name, no kick_template — so the ordinary resolution order never reached it and the contract was changeable only by rebuilding the binary. It now resolves through the same paths as every other template, so an operator-saved copy or a policies-checkout copy wins over the embedded default.

This is an extraction, and that is asserted rather than claimed

TestReviewerLaneTemplate_ByteIdenticalToBuilder renders the template and the builder it came from and compares them byte for byte, at both close-authority levels, reporting the first differing line on failure.

That test is the load-bearing one. Every line of this contract is a rule an agent follows against a human queue, so "the template looks right" is not a standard worth shipping — and the pre-existing TestBuildReviewerMessage_ContractAtL5 can only check the ~25 strings somebody thought to list. Those existing tests now exercise the template path too, since that is what buildReviewerMessage returns, which is the parity coverage step 3 of the dispatch asked for.

Three decisions deliberately stay in Go

The template is editable — that is the point of shipping one. These are not wording, so an override must not be able to reach them:

Decision Where Why
Is the lane awake? Go, before any template is read Below ACMM L5 the kick is a stand-down. An override must not wake a lane on a low-trust hive.
Is there work? Go, before any template is read An empty escalated queue is a stand-down. An override must not manufacture a contract with nothing to adjudicate.
May this agent close a PR? Go, rendered into ${REVIEWER_CLOSE_AUTHORITY} Closing is operator-only below ACMM L6. Whether an agent may close a human-queued PR is a trust decision.

So the worst an override can do is change the wording of a kick that was already going to be sent. All three are pinned by tests that install a rogue operator template — one that unconditionally issues a contract and offers gh pr close — and prove it cannot wake the lane, invent work, or grant close authority.

Two smaller decisions worth flagging

The work list is passed into the substitution, not recomputed inside it. buildReviewerMessage has already gated on that exact list; re-reading ci-failing.json during rendering would reopen a window where a rewrite between the two reads renders a full adjudication contract over an empty list. That needed a substituteTemplateWithVars variant taking caller-supplied ${VAR}s — in which the built-ins win a name clash, so no call site can redefine ${GH_AUTH}. A test pins that.

The compiled-in contract is kept as the resolution fallback. Step 4 of the dispatch says to remove it "only after template coverage is green"; I have kept it, because a stripped binary or an override that reads as empty must not leave the reviewer with no contract, which is worse than the previous wording. The parity test is what makes keeping both safe — they cannot drift without failing it. Removing it is a follow-up, not this PR.

Cluster claimed (files/functions)

  • src/pkg/policies/defaults/reviewer-lane.md (new) — the extracted contract.
  • src/pkg/scheduler/reviewer_lane.goreviewerLaneTemplate, reviewerKickHeader, renderReviewerLaneTemplate, reviewerCloseAuthority, and buildReviewerMessageHardcoded (the former body, unchanged text, now the fallback).
  • src/pkg/scheduler/scheduler.gosubstituteTemplateWithVars; substituteTemplateWithPolicy delegates to it with nil, so every existing caller is untouched.
  • src/pkg/scheduler/reviewer_lane_template_test.go (new, 6 tests), src/docs/agent-configuration.md, CHANGELOG.md.

No behaviour change: this is the "pure template extraction plus parity tests" scope the dispatch specified.

Validation

  • go build ./..., go vet ./pkg/scheduler/ ./pkg/policies/, gofmt -l clean on every file I touched. (gofmt flags three other pkg/scheduler test files — pre-existing drift, untouched here.)

  • go test ./pkg/scheduler/ ./pkg/policies/ — both packages pass in full, including every pre-existing reviewer-lane contract test.

  • src/scripts/check-docs-links.py: all links and anchors resolve.

  • Mutation-checked, and both mutations are caught by the new tests and by the pre-existing ones:

    Mutation Caught by
    Drop one INVARIANT clause from the template _ByteIdenticalToBuilder and TestBuildReviewerMessage_ContractAtL5
    Resolve the template before the ACMM gate _GatesAreNotTemplated and TestBuildReviewerMessage_ACMMGateDormantBelowL5 and _EmptyQueueStandsDown

Related issues

Refs #5617 (item 2 of 4 — items 1, 3 and 4 landed in #5634 and #5682). Related: #5480 (the reviewer lane), #5511 (the one-pass ladder the contract encodes).

Testing

  • cd src && go build ./...
  • cd src && go test ./... — ran ./pkg/scheduler/ and ./pkg/policies/ in full (the only packages touched); both pass.

— hive: backend=claude model=claude-opus-5

Item 2 of hivecommons#5617, the last of the four follow-ups deferred from the hivecommons#5480
delivery, and the one the dispatch on that issue asked for: extract
buildReviewerMessage into pkg/policies/defaults/reviewer-lane.md.

The lane reaches its prompt by ROLE, not by kick_template. An operator enables
it by adding a cadence agent with `role: reviewer` — any name, no
kick_template — so the ordinary resolution order never reached it and the
contract was changeable only by rebuilding the binary. It now resolves through
the same paths as every other template, so an operator-saved copy or a
policies-checkout copy wins over the embedded default.

This is an EXTRACTION, not a rewrite, and that is asserted rather than claimed:
TestReviewerLaneTemplate_ByteIdenticalToBuilder renders the template and the
builder it came from and compares them byte for byte, at both close-authority
levels. Every line of this contract is a rule an agent follows against a HUMAN
queue, so "looks right" is not a standard worth shipping — and the pre-existing
TestBuildReviewerMessage_ContractAtL5 can only check the ~25 strings somebody
thought to list. Those tests now exercise the template path too, since that is
what buildReviewerMessage returns.

Three decisions are deliberately NOT in the template, because the template is
editable and these are not wording:

  - Is the lane awake? Below ACMM L5 the kick is a stand-down. Evaluated in Go
    BEFORE any template is read, so an override cannot wake a lane on a
    low-trust hive.
  - Is there work? An empty escalated queue is a stand-down. Also evaluated
    first, so an override cannot manufacture a contract with nothing to
    adjudicate.
  - May this agent close a PR? Operator-only below ACMM L6. Computed in Go and
    rendered into ${REVIEWER_CLOSE_AUTHORITY} rather than expressed as template
    logic: whether an agent may close a human-queued PR is a trust decision.

So the worst an override can do is change the wording of a kick that was already
going to be sent. All three are pinned by tests that install a rogue operator
template and prove it cannot wake the lane, invent work, or grant close
authority.

The work list is passed INTO the substitution rather than recomputed inside it.
buildReviewerMessage has already gated on that exact list; re-reading
ci-failing.json during rendering would reopen a window where a rewrite between
the two reads renders a full adjudication contract over an empty list. That
needed a substituteTemplateWithVars variant taking caller-supplied ${VAR}s, in
which the built-ins win a name clash so no call site can redefine ${GH_AUTH}.

The compiled-in contract is kept as the resolution fallback — a stripped binary
or an override that reads as empty must not leave the reviewer with no contract
at all, which is worse than the previous wording. The parity test is what makes
keeping both safe: the two cannot drift without failing it.

Refs hivecommons#5617 (item 2 of 4 — the last outstanding one)

Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
agent-configuration.md's "Kick templates" section described a resolution order
that the reviewer lane does not use — it reaches its prompt by `role: reviewer`,
not by `kick_template` — so an operator reading it had no way to learn that the
lane's contract is now editable, or where it lives.

Adds that, and the part that actually matters: a table of the three decisions
that stay in Go and cannot be changed by editing the template — whether the lane
is awake, whether there is work, and whether the agent may close a PR — with why
each is a trust decision rather than wording. An operator who edits this file
should know exactly how much rope they have.

Also disambiguates it from `reviewer-advisory.md`, which belongs to the
pack-defined on-demand reviewer and never touches the needs-human queue. The two
route to entirely different agents.

Refs hivecommons#5617

Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
@kubestellar-prow kubestellar-prow Bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Sep 2, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign danathar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubestellar-prow kubestellar-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 2, 2026

@clubanderson clubanderson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the diff against the requested base in a detached worktree. Validation: cd src && go build ./... plus targeted package tests for changed Go packages (and relay tests where applicable) passed locally. I found no blocking issues.

@clubanderson

Copy link
Copy Markdown
Member

/lgtm

@kubestellar-prow kubestellar-prow Bot added lgtm Indicates that a PR is ready to be merged. labels Sep 3, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: c3a1021896f6447689c13676160878951b75f258

1 similar comment
@kubestellar-prow

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: c3a1021896f6447689c13676160878951b75f258

@clubanderson
clubanderson merged commit 4f9e7e1 into hivecommons:v5 Sep 3, 2026
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants